-
Notifications
You must be signed in to change notification settings - Fork 0
Feature | Add OpenAPI documentation controller OAuth2UserApiController v2 routes #105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature | Add OpenAPI documentation controller OAuth2UserApiController v2 routes #105
Conversation
Signed-off-by: matiasperrone-exo <[email protected]>
Signed-off-by: matiasperrone-exo <[email protected]>
…els schemas Signed-off-by: matiasperrone-exo <[email protected]>
Signed-off-by: matiasperrone-exo <[email protected]>
Signed-off-by: matiasperrone-exo <[email protected]>
martinquiroga-exo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: matiasperrone-exo <[email protected]>
… v2 routes Signed-off-by: matiasperrone-exo <[email protected]>
e06252a to
bba5793
Compare
caseylocker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- CRITICAL
app/Swagger/Security/UsersOAuth2Schema.php uses hardcoded relative paths:
authorizationUrl: '/oauth2/auth',
tokenUrl: '/oauth2/token',
Per project conventions like in summit-api, these must use the L5-Swagger constants:
authorizationUrl: L5_SWAGGER_CONST_AUTH_URL,
tokenUrl: L5_SWAGGER_CONST_TOKEN_URL,
The .env.example already defines L5_SWAGGER_CONST_AUTH_URL and L5_SWAGGER_CONST_TOKEN_URL, and the config registers them under defaults.constants. Hardcoding defeats the purpose of environment-specific URLs.
app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.php
The getV2 route in routes/api_v2.php is wrapped in service.account middleware:
Route::get('', ['middleware' => 'service.account', 'uses' => 'OAuth2UserApiController@getV2']);
The OpenAPI attribute on getV2 has summary: 'Get a user by ID' but no description mentioning the middleware requirement. Per conventions, it should include something like:
description: 'Requires service account authentication (middleware: service.account)',
UsersOAuth2Schema.php only defines one scope:
IUserScopes::ReadAll => 'Read All Users Data',
The security scheme definition should include all scopes that will be referenced across User endpoints (not just the one used by this PR's endpoint). Looking at IUserScopes, the scheme should also include at minimum: MeRead, Write, UserGroupWrite, and Registration — any scope that future User/Group endpoints will reference.
config/l5-swagger.php Copy/Paste
'title' => 'Summit API Swagger UI',
Should be 'OpenStackID API Swagger UI' or similar. This is a copy-paste from the summit-api project.
Task:
Ref: https://app.clickup.com/t/86b7fj5wh
Endpoints affected: